Do not install playwright in CI #12607
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thank you for the pull request, @jjspace! ✅ We can confirm we have a CLA on file for you. |
ggetz
requested changes
May 8, 2025
ggetz
reviewed
May 8, 2025
| const { env } = process; | ||
|
|
||
| const isCI = !!( | ||
| env.CI !== "false" && // Bypass all checks if CI env is explicitly set to 'false' |
There was a problem hiding this comment.
While I do appreciate the robust approach, why did we decide to cover all possible types of CI providers? Just remember that we'd need to maintain this script going forward, and I could see it becoming out of date quickly.
There was a problem hiding this comment.
@ggetz it was simply because the library I was copying from already had them all and it was the same amount of effort (right now) to copy all of them vs copying one. I can remove them if you think it's better for us long term to focus on our use case, can re-add if we ever need to
ggetz
approved these changes
May 8, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
When testing CI build support for #12574 I was running the CI builds a lot. I took a brief look into ways to speed up CI and realized we install all the playwright dependencies for every CI job even though none of them currently do anything with CI. On average the
npm installsteps seems to take~1m 20sin most jobs. Without doing theplaywright installstep this drops to only~20-30s. Compare thedeployjob in main to the deploy job on this branch's commitOnce we decide to move forward with #12355 we can force it to install specifically in the CI workflows it's needed.
As an aside I did spend a little time looking at turning on caching for the npm packages in CI. This seems like it would be really helpful but relies on a "cache key" that defines when to invalidate the cache. They heavily suggest using a
package-lock.jsonfile as that will always change with every dependency. However we don't use one so there's a decent chance it will not invalidate the cache when it actually should for version ranges inpackage.json. If we could find a way to set our own key based on the "wanted" values ofnpm outdatedI think that would be our "ideal" setup. That said the node action currently does not allow setting our own key anyway but there's a PR for it.On top of that the largest slowdown with the install step from Playwright is downloading the browser binaries and they specifically say that you should not cache these. Even if you did rebuilding them would take as much time as re-downloading.
Issue number and link
No issue
Testing plan
npm installlocally and make sure playwright does still install and download browsers locallyCI=true npm installto see that running it in CI does not do the playwright installAuthor checklist
CONTRIBUTORS.mdCHANGES.mdwith a short summary of my change